Skip to content

Update pyproject.toml license settings and minimum python version#191

Merged
Miauwkeru merged 10 commits intomainfrom
update-pyproject-toml-file
Sep 25, 2025
Merged

Update pyproject.toml license settings and minimum python version#191
Miauwkeru merged 10 commits intomainfrom
update-pyproject-toml-file

Conversation

@Miauwkeru
Copy link
Contributor

  • Change the minimum python version to 3.10
  • Change vermin minimum version to python3.10
  • Fix linting for python3.10

@Miauwkeru Miauwkeru requested a review from twiggler September 22, 2025 15:25
@codecov
Copy link

codecov bot commented Sep 22, 2025

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.32%. Comparing base (9daa5c9) to head (379d702).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
flow/record/adapter/xlsx.py 0.00% 1 Missing ⚠️
flow/record/fieldtypes/net/ipv4.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   83.35%   83.32%   -0.03%     
==========================================
  Files          35       35              
  Lines        3718     3713       -5     
==========================================
- Hits         3099     3094       -5     
  Misses        619      619              
Flag Coverage Δ
unittests 83.32% <86.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Miauwkeru Miauwkeru force-pushed the update-pyproject-toml-file branch 2 times, most recently from 0365f68 to 92c379a Compare September 23, 2025 14:04
@@ -39,10 +38,7 @@ def fresh_app_context() -> Generator[AppContext, None, None]:

# Use slots=True on dataclass for better performance which requires Python 3.10 or later.
# This can be removed when we drop support for Python 3.9.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment.

rec = TestRecordWithFooBar(name="world", foo="foo", bar="bar")
writer.write(rec)
out, err = capsys.readouterr()
out, _err = capsys.readouterr()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this? If it's unused, why not _?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new version of ruff adds a warning if a variable is unused. Prefixing it with a _ stops that warning, and you can still see what the variable was supposed to be

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use it so I don't think we care, just put _.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion having it as _err is fine, it's still useful for readability even when we don't use it.

for field_name in self.datetime_fields:
value = obj.get(field_name, None)
if isinstance(value, (int, float)) and value > 0xFFFFFFFF:
if isinstance(value, int | float) and value > 0xFFFFFFFF:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this was a bit slower compared to old style tuples. I'm not sure if that is true, but if that's the case I think we can keep it old style unless there is a good reason not to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruff is enforcing this now I think. So we'd have to disable that rule if we want to keep the old style.

I don't really like this new style either, to be honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule was already removed in the version of ruff we now use. It was an issue where the local ruff I used was older and still had the rule enabled. I reverted all those changes to the projects

@Miauwkeru Miauwkeru force-pushed the update-pyproject-toml-file branch from 92c379a to c10e69d Compare September 24, 2025 12:42
@Miauwkeru Miauwkeru requested a review from Schamper September 24, 2025 14:40

grouped = GroupedRecord("grouped/ab", [a, b])
assert isinstance(grouped, (Record, GroupedRecord))
assert isinstance(grouped, Record | GroupedRecord)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We reverted this no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I missed reverting this one

@Miauwkeru Miauwkeru requested a review from Schamper September 25, 2025 14:48
@Miauwkeru Miauwkeru merged commit f2606d1 into main Sep 25, 2025
21 checks passed
@Miauwkeru Miauwkeru deleted the update-pyproject-toml-file branch September 25, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants